Handle response content length mismatches (#175) by cesarblum · Pull Request #1155 · aspnet/KestrelHttpServer

cesarblum · 2016-10-07T17:24:26Z

TODO: InvalidOperationException might not be the right exception type for this
TODO: is logging this as application errors OK or should we add new event IDs for response length mismatches?

@halter73 @Tratcher @mikeharder @davidfowl

Tratcher · 2016-10-07T17:28:03Z

+                var responseHeaders = FrameResponseHeaders;
+                if (responseHeaders != null && responseHeaders.HasContentLength)
+                {
+                    _responseContentLength = int.Parse(responseHeaders.HeaderContentLength.ToString());


TryParse, culture, format

I'll leave it as Parse since that will throw if the app does something like httpContext.Response.Headers["Content-Length"] = "foo";

In that case you should throw from the header setter ideally.

I think we should make sure we have the same logic we use in HttpAbstractions. We should probably look into sharing this with HttpAbsractions somehow.

It's also a good opportunity to make sure we use the same logic for parsing the request header content-length. Right now, it seems like we're probably more lax than we should be.

Oh. And I see you changed this to a long 👍 Were there not any tests failing when this was int.Parse?

No test failures :( It was bound to blow up somewhere.

Wow. I thought our LargeDownload test wrote over 2GB like the LargeUpload test. It looks like the LargeDownload test only has a 1MB body.

We should change the LargeDownload test to be like the LargeUpload test where we check every byte value in a few megabyte scenario, but still ensure that sending over 2GB works.

Tratcher · 2016-10-07T17:29:06Z

+
+        protected void TryInitializeResponseContentLength()
+        {
+            if (!_responseContentLength.HasValue)


If there's no content-length header this will re-run for every write. You need a flag to only run it once, or is HasContentLength really that fast?

Tratcher · 2016-10-07T17:31:07Z

+
+                            if (_responseContentLength.HasValue && _responseBytesWritten < _responseContentLength.Value)
+                            {
+                                ThrowContentLengthMismatchException();


This should be a different message "Too few bytes written".

Why even throw at this point? Why not just directly invoke ReportApplicationError?

Why have two different messages when we can have a single one that reports the error in the same way?

because they are different conditions, and the message should be clear which is which.

Tratcher · 2016-10-07T17:31:55Z

+        protected void ThrowContentLengthMismatchException()
+        {
+            _keepAlive = false;
+            throw new InvalidOperationException($"Response content length mismatch: wrote {_responseBytesWritten} bytes to {_responseContentLength.Value}-byte response.");


Too many bytes written

Yes, Stream allows InvalidOperationException.
https://msdn.microsoft.com/en-us/library/hh137799(v=vs.110).aspx

Tratcher · 2016-10-07T17:34:10Z

            }

            _responseBytesWritten += data.Count;
+            VerifyWrite();


this calling pattern is misleading, _responseBytesWritten gets incremented regardless of if you've written the bytes out. Invert it:

VerifyWrite(data.Count); _responseBytesWritten += data.Count;

and/or move _responseBytesWritten += data.Count; inside of VerifyWrite and make it VerifyAndUpdateWrite

cesarblum · 2016-10-07T18:27:53Z

@Tratcher I've addressed your comments.

Tratcher · 2016-10-07T18:39:33Z

+                var responseHeaders = FrameResponseHeaders;
+                if (responseHeaders != null && responseHeaders.HasContentLength)
+                {
+                    _responseContentLength = long.Parse(responseHeaders.HeaderContentLength.ToString(), NumberStyles.None);


This will result in a FormatException being thrown from Write. Use TryParse and ignore values you can't parse.
When determining if you should chunk, do you check for the presence of a content-length or if you understand it?

We check for its presence but we don't try to parse it.

I disagree with you on this point. We should throw if the Content-Length header is set to an invalid value. The whole point of this change is to track errors related to response Content-Length, this would be one of them. Without a valid value, we can't track it.

Even if you want to throw, you can't throw a FormatException from Stream.Write. It needs to be wrapped in an InvalidOperationException

cesarblum · 2016-10-07T20:18:34Z

Updated.

cesarblum · 2016-10-10T18:54:41Z

Added validation to header setter. Not sure I covered all bases though. I'm thinking of adding a long _contentLength field to FrameHeaders, this would give fast access to that value instead of having to parse it from HeaderContentLength.

Tratcher · 2016-10-10T18:55:05Z

            }
            set
            {
+                ValidateLongValue(value);


can you cache the parsed value internally rather than re-parse it later?

Yeah, it's what I was referring to in my comment above.

halter73 · 2016-10-10T19:11:57Z


+        public static void ValidateLongValue(StringValues value)
+        {
+            long.Parse(value, NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, CultureInfo.InvariantCulture);


Might make things a little easier to debug if this was wrapped in an InvalidOperationException stating the Content-Length header must be a positive decimal number or something like this.

cesarblum · 2016-10-10T19:43:58Z

Updated. Caching Content-Length and wrapping FormatException in InvalidOperationException.

Tratcher · 2016-10-10T20:47:57Z


        private long _bits = 0;
        private HeaderReferences _headers;
+        private long? _contentLength;


Does this get cleared if I call Headers.Remove("Content-Length")?

Good catch. We should add a test case for this.

Done a7e7044

cesarblum · 2016-10-10T21:58:09Z

I'm applying the FrameHeaders change to FrameResponseHeaders only (1d3b71d)

Tratcher · 2016-10-10T22:03:02Z

+                        "GET / HTTP/1.1",
+                        "",
+                        "");
+                    await connection.ReceiveEnd(


Would this result in a RST? or just a FIN?

Tratcher · 2016-10-10T22:05:59Z

+
+            using (var server = new TestServer(httpContext =>
+            {
+                httpContext.Response.ContentLength = 0;


What if you added a version of this test that called httpContext.Respose.Body.Flush() here? Same result I hope.

Same result. I've modified this test to test with and without flushing.

halter73 · 2016-10-10T22:15:05Z

                        {{
                            if ({header.TestBit()})
-                            {{
+                            {{{If(header.Identifier == "ContentLength", () => $@"


Missed loop.ClassName == "FrameResponseHeaders" && here.

halter73 · 2016-10-10T22:17:15Z

        {{
            _bits = 0;
            _headers = default(HeaderReferences);
+            _contentLength = null;


Missed If here too.

halter73 · 2016-10-10T22:27:47Z

        protected Task TryProduceInvalidRequestResponse()
        {
-            if (_requestRejected)
+            if (_requestRejected || _applicationException != null)


Why is this change necessary?

Obviously the application could swallow the exception, so we shouldn't be relying on it. And this change has little to do with invalid requests, right? I guess we're now parsing the request content-length more strictly...

If the response hasn't already started by the time we detect a mismatch in the content length, we can send a 500 response. _applicationException is set when there's a mismatch, so we need this check to run the error response logic.

That should be handled by the normal call to ProduceEnd().

Indeed. My very first change (that I didn't even commit) was calling Abort() on a content length mismatch. This would prevent the call to ProduceEnd() in FrameOfT. But since I moved away from that decision, ProduceEnd() handles it as expected.

halter73 · 2016-10-10T22:40:55Z

I don't think we need a new event id for this. Treating it as a normal application error is fine with me.

cesarblum · 2016-10-10T22:57:47Z

Updated.

halter73 · 2016-10-11T17:26:25Z

halter73 · 2016-10-11T17:28:35Z

                _headers._raw{header.Identifier} = null;")}
            }}
        }}")}
+        {(loop.ClassName == "FrameResponseHeaders" ? "public long? HeaderContentLengthValue => _contentLength;" : "")}


nit: I would probably put this in FrameResponseHeaders.cs

halter73 · 2016-10-11T17:30:25Z

            return $@"
 using System;
 using System.Collections.Generic;
+using System.Globalization;


nit: I don't think this is being used.

halter73 · 2016-10-11T17:32:37Z

                    {{{Each(byLength, header => $@"
                        if (""{header.Name}"".Equals(key, StringComparison.OrdinalIgnoreCase))
-                        {{
+                        {{{If(loop.ClassName == "FrameResponseHeaders" && header.Identifier == "ContentLength", () => $@"


nit: $ is unnecessary.

halter73 · 2016-10-11T17:33:17Z

-                            }}
+                            }}{
+                            If(loop.ClassName == "FrameResponseHeaders" && header.Identifier == "ContentLength", () => $@"
+                            _contentLength = ParseContentLength(value);")}


No $ needed

halter73 · 2016-10-11T17:33:48Z

            }}
            set
-            {{
+            {{{If(loop.ClassName == "FrameResponseHeaders" && header.Identifier == "ContentLength", () => $@"


halter73 · 2016-10-11T17:34:24Z

                        {{
                            if ({header.TestBit()})
-                            {{
+                            {{{If(loop.ClassName == "FrameResponseHeaders" && header.Identifier == "ContentLength", () => $@"


cesarblum · 2016-10-11T17:35:18Z

@halter73 nits addressed :)

dnfclas added the cla-already-signed label Oct 7, 2016

cesarblum force-pushed the cesarbs/track-response-content-length branch from 9c4f978 to 83a49d0 Compare October 7, 2016 17:25

Tratcher reviewed Oct 7, 2016

View reviewed changes

Tratcher reviewed Oct 10, 2016

View reviewed changes

halter73 reviewed Oct 10, 2016

View reviewed changes

cesarblum force-pushed the cesarbs/track-response-content-length branch from 1aa76e4 to a837285 Compare October 10, 2016 19:45

Tratcher reviewed Oct 10, 2016

View reviewed changes

cesarblum force-pushed the cesarbs/track-response-content-length branch from a837285 to a7e7044 Compare October 10, 2016 21:57

Tratcher reviewed Oct 10, 2016

View reviewed changes

halter73 reviewed Oct 10, 2016

View reviewed changes

cesarblum force-pushed the cesarbs/track-response-content-length branch from 8e1eb31 to 6412753 Compare October 11, 2016 17:22

halter73 reviewed Oct 11, 2016

View reviewed changes

halter73 approved these changes Oct 11, 2016

View reviewed changes

Handle response content length mismatches (#175).

f8813a6

cesarblum force-pushed the cesarbs/track-response-content-length branch from 2e6ffce to f8813a6 Compare October 11, 2016 20:25

cesarblum merged commit f8813a6 into dev Oct 11, 2016

cesarblum deleted the cesarbs/track-response-content-length branch October 11, 2016 20:32

Uh oh!

Conversation

cesarblum commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum commented Oct 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum commented Oct 7, 2016

Uh oh!

cesarblum commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cesarblum commented Oct 7, 2016 •

edited

Loading

cesarblum Oct 10, 2016 •

edited

Loading